Skip to content

Fix AE isPluggableDataformatIndex for multi index PPL queries#5581

Open
finnegancarroll wants to merge 1 commit into
opensearch-project:mainfrom
finnegancarroll:analytics-engine-security-it-wildcards
Open

Fix AE isPluggableDataformatIndex for multi index PPL queries#5581
finnegancarroll wants to merge 1 commit into
opensearch-project:mainfrom
finnegancarroll:analytics-engine-security-it-wildcards

Conversation

@finnegancarroll

@finnegancarroll finnegancarroll commented Jun 24, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Description

Related to opensearch-project/OpenSearch#22314 where AE parsing of indices "index1, index2" would fail silently, subjecting the request to an FGAC check with no indices provided.

This PR adds the SQL side regression tests verifying that FGAC correctly evaluates permissions on all indices in comma-separated multi-source PPL queries.

Some cases covered here:

  • testPPLMultiIndexDeniedWhenSecondIndexUnauthorized — source = allowed, forbidden → 403
  • testPPLMultiIndexDeniedWithBackticksAuthorizedFirst — source = \allowed, forbidden`` → 403
  • testPPLMultiIndexDeniedWithUnauthorizedFirst — source = forbidden, allowed → 403
  • testPPLMultiIndexAllowedWhenAllAuthorized — source = allowed1, allowed2 → 200
  • testPPLDoubleCommaRejected — source = idx1,,idx2 → 400
  • testPPLLeadingCommaRejected — source = ,idx1 → 400
  • testPPLTrailingCommaRejected — source = idx1, → 400
  • testSQLMultiIndexCommaInFromRejected — SELECT name, age FROM idx1,idx2 LIMIT 3 → 400
  • testSQLMultiIndexCrossJoinRejected — SELECT a.name FROM idx1 a, idx2 b LIMIT 3 → 400
  • testSQLMultiIndexJoinRejected — SELECT a.name FROM idx1 a JOIN idx2 b ON a.name = b.name LIMIT 3 → 400

Also fixes

Plugin install order in analyticsEngineSecurityIT cluster config — composite-engine must be installed before analytics-backend-lucene (dependency).

@finnegancarroll finnegancarroll force-pushed the analytics-engine-security-it-wildcards branch 15 times, most recently from 38a327d to bc2577c Compare June 26, 2026 18:00
@github-actions

github-actions Bot commented Jun 26, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

(Review updated until commit 3abf88e)

Here are some key observations to aid the review process:

🧪 PR contains tests
🔒 Security concerns

FGAC bypass:
The allIndicesArePluggableDataformat method returns true when given an input like ", , " (only commas/whitespace). This allows a query with no actual indices to be routed to the analytics engine path, potentially bypassing fine-grained access control checks that expect at least one valid index. The method should return false if no non-empty index is found after trimming.

✅ No TODO sections
🔀 No multiple PR themes
⚡ Recommended focus areas for review

Empty index bypass

When all indices in a comma-separated list are empty strings (e.g., "source = , , "), the method returns true, incorrectly treating the query as targeting pluggable dataformat indices. This occurs because the loop skips empty trimmed strings with 'continue' but never sets a flag indicating that at least one valid index was found. An attacker could exploit this to bypass FGAC checks by providing only whitespace/commas.

 
private boolean allIndicesArePluggableDataformat(String indexExpression) {
  String[] indices = indexExpression.split(",");
  if (indices.length == 0) {
    return false;
  }
  for (String idx : indices) {
    String trimmed = idx.trim();
    if (trimmed.isEmpty()) {
      continue;
    }
    if (!isPluggableDataformatIndex(trimmed)) {
      return false;
    }
  }
  return true;
}

@github-actions

github-actions Bot commented Jun 26, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Latest suggestions up to 3abf88e

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Security
Validate non-empty index exists

The method returns true when all indices are empty strings after trimming (e.g., "
, , "). This could bypass security checks. Add validation to ensure at least one
non-empty index exists before returning true.

plugin/src/main/java/org/opensearch/sql/plugin/rest/RestUnifiedQueryAction.java [137-152]

 private boolean allIndicesArePluggableDataformat(String indexExpression) {
   String[] indices = indexExpression.split(",");
   if (indices.length == 0) {
     return false;
   }
+  boolean hasNonEmptyIndex = false;
   for (String idx : indices) {
     String trimmed = idx.trim();
     if (trimmed.isEmpty()) {
       continue;
     }
+    hasNonEmptyIndex = true;
     if (!isPluggableDataformatIndex(trimmed)) {
       return false;
     }
   }
-  return true;
+  return hasNonEmptyIndex;
 }
Suggestion importance[1-10]: 9

__

Why: This is a critical security issue. The current implementation returns true when all indices are empty strings, which could bypass security checks in isAnalyticsIndex(). The suggestion correctly identifies that at least one non-empty index must be validated before returning true.

High

Previous suggestions

Suggestions up to commit 823030c
CategorySuggestion                                                                                                                                    Impact
Security
Validate non-empty indices exist

The method returns true when all indices are empty strings after trimming (e.g., "
, , "). This could bypass security checks. Add validation to ensure at least one
non-empty index exists before returning true.

plugin/src/main/java/org/opensearch/sql/plugin/rest/RestUnifiedQueryAction.java [137-152]

 private boolean allIndicesArePluggableDataformat(String indexExpression) {
   String[] indices = indexExpression.split(",");
   if (indices.length == 0) {
     return false;
   }
+  boolean hasNonEmptyIndex = false;
   for (String idx : indices) {
     String trimmed = idx.trim();
     if (trimmed.isEmpty()) {
       continue;
     }
+    hasNonEmptyIndex = true;
     if (!isPluggableDataformatIndex(trimmed)) {
       return false;
     }
   }
-  return true;
+  return hasNonEmptyIndex;
 }
Suggestion importance[1-10]: 9

__

Why: Critical security issue: the method currently returns true when all indices are empty strings (e.g., " , , "), which could bypass security checks. The suggestion correctly identifies that at least one non-empty index must exist before returning true, preventing potential FGAC bypass vulnerabilities.

High
Suggestions up to commit 6b42b0c
CategorySuggestion                                                                                                                                    Impact
Security
Prevent empty index list bypass

The method returns true when all indices are empty strings after trimming (e.g., " ,
, "). This bypasses security checks. Add validation to ensure at least one non-empty
index exists before returning true.

plugin/src/main/java/org/opensearch/sql/plugin/rest/RestUnifiedQueryAction.java [137-152]

 private boolean allIndicesArePluggableDataformat(String indexExpression) {
   String[] indices = indexExpression.split(",");
   if (indices.length == 0) {
     return false;
   }
+  boolean hasValidIndex = false;
   for (String idx : indices) {
     String trimmed = idx.trim();
     if (trimmed.isEmpty()) {
       continue;
     }
+    hasValidIndex = true;
     if (!isPluggableDataformatIndex(trimmed)) {
       return false;
     }
   }
-  return true;
+  return hasValidIndex;
 }
Suggestion importance[1-10]: 9

__

Why: This is a critical security vulnerability. The current implementation returns true when all indices are empty strings (e.g., "source = , , "), which could bypass authorization checks. The suggestion correctly identifies this flaw and provides a proper fix by tracking whether at least one valid index was found before returning true.

High
Suggestions up to commit 6b42b0c
CategorySuggestion                                                                                                                                    Impact
Possible issue
Handle all-empty index expressions correctly

The method returns true when all indices are empty strings after trimming (e.g.,
indexExpression = " , , "). This edge case should return false since no valid
indices exist. Track whether at least one valid index was checked before returning
true.

plugin/src/main/java/org/opensearch/sql/plugin/rest/RestUnifiedQueryAction.java [138-152]

 String[] indices = indexExpression.split(",");
 if (indices.length == 0) {
   return false;
 }
+boolean hasValidIndex = false;
 for (String idx : indices) {
   String trimmed = idx.trim();
   if (trimmed.isEmpty()) {
     continue;
   }
+  hasValidIndex = true;
   if (!isPluggableDataformatIndex(trimmed)) {
     return false;
   }
 }
-return true;
+return hasValidIndex;
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies a logic bug where an input like " , , " would incorrectly return true despite having no valid indices. The proposed fix using hasValidIndex flag ensures at least one non-empty index was validated before returning true, preventing potential security or correctness issues.

Medium
Suggestions up to commit e00e1e7
CategorySuggestion                                                                                                                                    Impact
Possible issue
Handle all-empty indices edge case

The method returns true when all indices are empty strings after trimming (e.g., " ,
, "). This edge case should return false since no valid indices exist. Track whether
at least one valid index was found before returning true.

plugin/src/main/java/org/opensearch/sql/plugin/rest/RestUnifiedQueryAction.java [137-152]

 private boolean allIndicesArePluggableDataformat(String indexExpression) {
   String[] indices = indexExpression.split(",");
   if (indices.length == 0) {
     return false;
   }
+  boolean hasValidIndex = false;
   for (String idx : indices) {
     String trimmed = idx.trim();
     if (trimmed.isEmpty()) {
       continue;
     }
+    hasValidIndex = true;
     if (!isPluggableDataformatIndex(trimmed)) {
       return false;
     }
   }
-  return true;
+  return hasValidIndex;
 }
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies a logical flaw where the method returns true when all indices are empty strings after trimming. The improved code adds a hasValidIndex flag to ensure at least one valid index exists before returning true, which is a meaningful correctness improvement.

Medium
Suggestions up to commit 84988c6
CategorySuggestion                                                                                                                                    Impact
Possible issue
Handle all-empty indices edge case

The method returns true when all indices are empty strings after trimming (e.g., "
, , "). This edge case should return false since no valid indices were found.
Track whether at least one valid index was checked before returning true.

plugin/src/main/java/org/opensearch/sql/plugin/rest/RestUnifiedQueryAction.java [137-152]

 private boolean allIndicesArePluggableDataformat(String indexExpression) {
   String[] indices = indexExpression.split(",");
   if (indices.length == 0) {
     return false;
   }
+  boolean foundValidIndex = false;
   for (String idx : indices) {
     String trimmed = idx.trim();
     if (trimmed.isEmpty()) {
       continue;
     }
+    foundValidIndex = true;
     if (!isPluggableDataformatIndex(trimmed)) {
       return false;
     }
   }
-  return true;
+  return foundValidIndex;
 }
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies a logical edge case where an input like " , , " would return true despite containing no valid indices. The foundValidIndex flag ensures at least one non-empty index was validated before returning true, improving correctness.

Medium

@finnegancarroll finnegancarroll force-pushed the analytics-engine-security-it-wildcards branch from bc2577c to 84988c6 Compare June 26, 2026 18:54
@github-actions

github-actions Bot commented Jun 26, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit 3abf88e.

PathLineSeverityDescription
plugin/src/main/java/org/opensearch/sql/plugin/rest/RestUnifiedQueryAction.java147lowIn allIndicesArePluggableDataformat, empty or whitespace-only index tokens are silently skipped via 'continue', causing an all-empty expression (e.g., "," or " ") to fall through and return true. This could misroute queries to the analytics engine for degenerate inputs, though FGAC enforcement is handled separately and is not bypassed by this routing decision.

The table above displays the top 10 most important findings.

Total: 1 | Critical: 0 | High: 0 | Medium: 0 | Low: 1

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

@github-actions

github-actions Bot commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 84988c6

@finnegancarroll finnegancarroll mentioned this pull request Jun 26, 2026
Merged
@finnegancarroll finnegancarroll force-pushed the analytics-engine-security-it-wildcards branch from 84988c6 to e00e1e7 Compare June 26, 2026 19:12
@finnegancarroll finnegancarroll self-assigned this Jun 26, 2026
@finnegancarroll finnegancarroll added bugFix testing Related to improving software testing labels Jun 26, 2026
@github-actions

github-actions Bot commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit e00e1e7

@finnegancarroll finnegancarroll force-pushed the analytics-engine-security-it-wildcards branch 2 times, most recently from 0b2f000 to 6b42b0c Compare June 26, 2026 19:26
@github-actions

github-actions Bot commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 6b42b0c

1 similar comment
@github-actions

github-actions Bot commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 6b42b0c

@finnegancarroll finnegancarroll force-pushed the analytics-engine-security-it-wildcards branch from 6b42b0c to 823030c Compare June 26, 2026 20:54
@github-actions

github-actions Bot commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 823030c

@finnegancarroll
Fix multi-index FGAC routing and add bypass regression tests
3abf88e 
SQL plugin routing fix:
- RestUnifiedQueryAction.isAnalyticsIndex() now splits comma-separated
  index names and checks each independently. Routes to analytics engine
  only if ALL indices are composite. Previously, the joined string
  'idx1,idx2' was looked up as a single index in cluster metadata,
  causing multi-index queries to fall through to the legacy pipeline.

Regression tests:
- testPPLMultiIndexDeniedWhenSecondIndexUnauthorized
- testPPLMultiIndexDeniedWithBackticksAuthorizedFirst
- testPPLMultiIndexDeniedWithUnauthorizedFirst
- testPPLMultiIndexAllowedWhenAllAuthorized

Also fixes plugin install order (composite-engine before backends).

Signed-off-by: Finnegan Carroll <carrofin@amazon.com>
Signed-off-by: Finn Carroll <carrofin@amazon.com>
@finnegancarroll finnegancarroll force-pushed the analytics-engine-security-it-wildcards branch from 823030c to 3abf88e Compare June 26, 2026 21:05
@github-actions

github-actions Bot commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 3abf88e

@finnegancarroll finnegancarroll marked this pull request as ready for review June 26, 2026 22:39
@finnegancarroll finnegancarroll changed the title Add FGAC integration tests for multi-index PPL queries Fix AE isPluggableDataformatIndex for multi index PPL queries Jun 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ps48 ps48 Awaiting requested review from ps48 ps48 is a code owner
@joshuali925 joshuali925 Awaiting requested review from joshuali925 joshuali925 is a code owner
@dai-chen dai-chen Awaiting requested review from dai-chen dai-chen is a code owner
@mengweieric mengweieric Awaiting requested review from mengweieric mengweieric is a code owner
@vamsimanohar vamsimanohar Awaiting requested review from vamsimanohar vamsimanohar is a code owner
@Swiddis Swiddis Awaiting requested review from Swiddis Swiddis is a code owner
@penghuo penghuo Awaiting requested review from penghuo penghuo is a code owner
@anirudha anirudha Awaiting requested review from anirudha anirudha is a code owner
@acarbonetto acarbonetto Awaiting requested review from acarbonetto acarbonetto is a code owner
@ykmr1224 ykmr1224 Awaiting requested review from ykmr1224 ykmr1224 is a code owner
@LantaoJin LantaoJin Awaiting requested review from LantaoJin LantaoJin is a code owner
@noCharger noCharger Awaiting requested review from noCharger noCharger is a code owner
@qianheng-aws qianheng-aws Awaiting requested review from qianheng-aws qianheng-aws is a code owner
@yuancu yuancu Awaiting requested review from yuancu yuancu is a code owner
@RyanL1997 RyanL1997 Awaiting requested review from RyanL1997 RyanL1997 is a code owner
@ahkcs ahkcs Awaiting requested review from ahkcs ahkcs is a code owner
@songkant-aws songkant-aws Awaiting requested review from songkant-aws songkant-aws is a code owner

At least 1 approving review is required to merge this pull request.

Assignees

@finnegancarroll finnegancarroll

Labels

bugFix testing Related to improving software testing

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@finnegancarroll